-
Notifications
You must be signed in to change notification settings - Fork 327
Allow configuring ochttp.Handler for public servers #563
Allow configuring ochttp.Handler for public servers #563
Conversation
I think we should similarly give user a way to label a public server instead of dropping default propagation support. By default, we don't want to drop the traces. |
f883749
to
0d1a33f
Compare
8ed1dd7
to
c996638
Compare
@rakyll updated PTAL |
Added a flag on ochttp.Handler that causes the incoming SpanContext to be added as a link rather than a parent. This is useful if we don't trust the metadata.
c996638
to
b0a41c6
Compare
plugin/ochttp/trace_test.go
Outdated
trace.SetDefaultSampler(trace.AlwaysSample()) | ||
|
||
cases := []struct { | ||
Name string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to export these fields.
name string
plugin/ochttp/trace_test.go
Outdated
|
||
cases := []struct { | ||
Name string | ||
ServerMiddleware *Handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handler
plugin/ochttp/trace_test.go
Outdated
cases := []struct { | ||
Name string | ||
ServerMiddleware *Handler | ||
ClientMiddleware *Transport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transport
plugin/ochttp/trace_test.go
Outdated
Name string | ||
ServerMiddleware *Handler | ||
ClientMiddleware *Transport | ||
ExpectSameTraceID bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wantSameTraceID
plugin/ochttp/trace_test.go
Outdated
ServerMiddleware *Handler | ||
ClientMiddleware *Transport | ||
ExpectSameTraceID bool | ||
ExpectLinks bool // expect a link between client and server span |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wantLinks
plugin/ochttp/trace_test.go
Outdated
} | ||
|
||
serverReturn <- time.Now().Add(time.Millisecond) | ||
for _, c := range cases { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Go, it is canonical to name cases tc, and specific case tt.
It improves readability to use tt because everyone knows it represents the current test case without having to jump to the declaration.
plugin/ochttp/trace_test.go
Outdated
req = req.WithContext(ctx) | ||
resp, err := c.ClientMiddleware.RoundTrip(req) | ||
if err != nil { | ||
t.Fatalf("unexpected error %s", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just t.Fatal(err)
plugin/ochttp/trace_test.go
Outdated
t.Fatalf("unexpected error %s", err) | ||
} | ||
if resp.StatusCode != http.StatusOK { | ||
t.Fatalf("unexpected stats: %d", resp.StatusCode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.Fatalf("resp.StatusCode = %v, ...)
The unexpected terminology doesn't exist in Go.
plugin/ochttp/trace_test.go
Outdated
if err != nil { | ||
t.Fatalf("unexpected read error: %#v", err) | ||
} | ||
if string(respBody) != "expected-response" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if got, want := string(respBody), "expected"; got != want {
t.Fatalf("Response body = %q; want %q", got, want)
}
plugin/ochttp/server.go
Outdated
ctx := r.Context() | ||
var span *trace.Span | ||
if sc, ok := p.SpanContextFromRequest(r); ok { | ||
sc, haveSC := h.extractSpanContext(r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sc, ok := h..extractSpanContext(r)
No description provided.